Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: cherry-pick dbfe4a49d8 from upstream V8 #16889

Closed
wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 8, 2017

Original commit message:

Introduce ScriptOrModule and HostDefinedOptions

This patch introduces a new container type ScriptOrModule which
provides the name and the host defined options of the script/module.

This patch also introduces a new PrimitivesArray that can hold
Primitive values, which the embedder can use to store metadata.

The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
passed back to the embedder through HostImportModuleDynamically for
module loading.

Bug: v8:5785, v8:6658, v8:6683
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
Reviewed-on: https://chromium-review.googlesource.com/622158
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47724}

Refs: v8/v8@dbfe4a4
Refs: #15713

/cc @nodejs/v8

Checklist
  • make -j4 test-only passes - something's weird about the doc & lint test setup on master
  • commit message follows commit guidelines
Affected core subsystem(s)

V8

@jkrems jkrems added esm Issues and PRs related to the ECMAScript Modules implementation. v8 engine Issues and PRs related to the V8 dependency. labels Nov 8, 2017
@jkrems
Copy link
Contributor Author

jkrems commented Nov 8, 2017

The patch did apply cleanly to everything but deps/v8/tools/v8heapconst.py and I wasn't sure how to regenerate that file..? I can build node and run all tests but I have no idea if that file matters at runtime or somewhere in our build process.

@jkrems jkrems mentioned this pull request Nov 8, 2017
9 tasks
@hashseed
Copy link
Member

hashseed commented Nov 8, 2017

I don't think Node.js tests v8heapconst.py. Just for the record though: build for x64 release build and run mkgrokdump.

@jkrems
Copy link
Contributor Author

jkrems commented Nov 8, 2017

The "build" part is where I failed. I'm sure I'm just missing something basic here. I tried building the way I would in a V8 checkout and don't get past the initial step:

# Running inside deps/v8
> gn gen out.gn
gn.py: Could not find checkout in any parent of the current path.
This must be run inside a checkout.

The node build itself doesn't seem to include mkgrokdump (find ./out | grep mkgrokdump).

@fhinkel
Copy link
Member

fhinkel commented Nov 8, 2017

Thanks! Can you update the v8_embedder_string number in common.gypi, please. See backporting guide.

@jkrems
Copy link
Contributor Author

jkrems commented Nov 8, 2017

Ah, I saw it in the guide but then forgot about it for some reason. v8_embedder_string is updated now.

@targos
Copy link
Member

targos commented Nov 8, 2017

@fhinkel
Copy link
Member

fhinkel commented Nov 9, 2017

Did you run git commit --amend --reset-author? Usually we set the author of cherry-picks to the person back-porting, not the original author. Though I'm sure @gsathya doesn't mind.

@hashseed
Copy link
Member

hashseed commented Nov 9, 2017

You would need to run gclient sync to build with gn.

@jkrems jkrems force-pushed the jk-module-or-script-dbfe4a49d8 branch from 23bd3b7 to 3727303 Compare November 9, 2017 14:46
@jkrems
Copy link
Contributor Author

jkrems commented Nov 9, 2017

@fhinkel Thanks. I should really work on my reading comprehension. :D Did a reset of the author so it's consistent with past backports.

@fhinkel
Copy link
Member

fhinkel commented Nov 9, 2017

No worries. That document got really long and it's hard to spot and remember all the steps.

@targos
Copy link
Member

targos commented Nov 9, 2017

it's hard to spot and remember all the steps.

That's why I made an automated tool ;)

@jkrems
Copy link
Contributor Author

jkrems commented Nov 9, 2017

Heh, I think I got confused by its name. Next time!

@jkrems jkrems requested a review from bmeck November 9, 2017 16:37
@bmeck
Copy link
Member

bmeck commented Nov 9, 2017

Looks fine to me on first glance, will try and get to full check tonight

@fhinkel
Copy link
Member

fhinkel commented Nov 9, 2017

@targos Life changing! Thanks 🙏

@BridgeAR
Copy link
Member

Seems like this needs a rebase and is otherwise fine?

@BridgeAR
Copy link
Member

@nodejs/v8 PTAL

@bnoordhuis
Copy link
Member

This patch is already in 6.2, isn't it? We just need to upgrade to >= 6.2.436.

@jkrems
Copy link
Contributor Author

jkrems commented Nov 24, 2017

@bnoordhuis That was my initial assumption but @targos suggested that we wouldn't get 6.2.436 because the branch was cut at an earlier version of 6.2:

Then you're waiting for 6.3 because the 6.2 branch was cut at an earlier minor version.
If that commit alone is enough, an option is to cherry-pick it.

See #15713 (comment)

If we're fine pulling in a version of 6.2 after the stable branch was cut(*), then this cherry-pick isn't necessary. My motivation here is to have a path towards dynamic import in node 8 (experimental) so we can continue fleshing out the module story.

P.S.: Punting on the rebase until there's agreement. So far it only affects the embedder string, so it's pretty straightforward to resolve when the time comes.

(*) This is based on my understanding that there's one specific 6.2.x that is considered the stable branch for 6.2 and a couple of 6.2.(x+n)s that were released later on but are pretty much "dead".

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. LGTM.

Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#47724}

Refs: v8/v8@dbfe4a4
Refs: nodejs#15713
@targos
Copy link
Member

targos commented Nov 25, 2017

IMHO this is good to go after running the regular pull request CI

@jkrems
Copy link
Contributor Author

jkrems commented Nov 25, 2017

@jkrems
Copy link
Contributor Author

jkrems commented Nov 25, 2017

Landed as 3a4fe77

@jkrems jkrems closed this Nov 25, 2017
jkrems pushed a commit that referenced this pull request Nov 25, 2017
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jkrems jkrems deleted the jk-module-or-script-dbfe4a49d8 branch November 25, 2017 20:26
@targos
Copy link
Member

targos commented Dec 6, 2017

Should this land on v9.x? I'm afraid it might be ABI incompatible.

@jkrems
Copy link
Contributor Author

jkrems commented Dec 6, 2017

@targos It would be great if we could land it on 9.x but I'm not sure how to verify that it's ABI compatible. I would assume that the change of the dynamic import callback signature wouldn't count since it only affects an unshipped feature..?

@targos
Copy link
Member

targos commented Dec 6, 2017

The signature of ScriptOrigin's constructor changed. I don't know if it's something that might be used.

@bnoordhuis
Copy link
Member

It can't land in any of the stable branches for the reason @targos mentions; ScriptOrigin is used by some add-ons.

@jkrems
Copy link
Contributor Author

jkrems commented Dec 6, 2017

Gotcha, thanks! Given that this only enables a feature that's still flagged anyhow in all stable versions, I assume even if we could create a patch to make it ABI compatible, it wouldn't be worth it.

jkrems pushed a commit to jkrems/node that referenced this pull request Feb 1, 2018
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#47724}

PR-URL: nodejs#16889
Refs: v8/v8@dbfe4a4
Refs: nodejs#15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins added a commit that referenced this pull request Feb 21, 2018
Notable changes:

* async_hooks:
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
  - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise
    (Ali Ijaz Sheikh) #18633
* deps:
  - update node-inspect to 1.11.3 (Jan Krems)
    #18354
  - ICU 60.2 bump (Steven R. Loomis)
    #17687
  - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems)
    #16889
* http:
  - add options to http.createServer() for `IncomingMessage` and
    `ServerReponse` (Peter Marton)
    #15752
* http2:
  - add http fallback options to .createServer (Peter Marton)
    #15752
* https:
  - Adds the remaining options from tls.createSecureContext() to the
    string generated by Agent#getName(). This allows https.request()
    to accept the options and generate unique sockets appropriately.
    (Jeff Principe)
    #16402
* inspector:
  - --inspect-brk for es modules (Guy Bedford)
    #18194
* lib:
  - allow process kill by signal number (Sam Roberts)
    #16944
* module:
  - enable dynamic import (Myles Borins)
    #18387
  - dynamic import is now supported (Jan Krems)
    #15713
* napi:
  - add methods to open/close callback scope (Michael Dawson)
    #18089
* src:
  - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko)
    #17600
* vm:
  - add support for es modules (Gus Caplan)
    #17560

PR-URL: #18902
MylesBorins added a commit that referenced this pull request Feb 21, 2018
Notable changes:

* async_hooks:
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
  - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise
    (Ali Ijaz Sheikh) #18633
* deps:
  - update node-inspect to 1.11.3 (Jan Krems)
    #18354
  - ICU 60.2 bump (Steven R. Loomis)
    #17687
  - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems)
    #16889
* http:
  - add options to http.createServer() for `IncomingMessage` and
    `ServerReponse` (Peter Marton)
    #15752
* http2:
  - add http fallback options to .createServer (Peter Marton)
    #15752
* https:
  - Adds the remaining options from tls.createSecureContext() to the
    string generated by Agent#getName(). This allows https.request()
    to accept the options and generate unique sockets appropriately.
    (Jeff Principe)
    #16402
* inspector:
  - --inspect-brk for es modules (Guy Bedford)
    #18194
* lib:
  - allow process kill by signal number (Sam Roberts)
    #16944
* module:
  - enable dynamic import (Myles Borins)
    #18387
  - dynamic import is now supported (Jan Krems)
    #15713
* napi:
  - add methods to open/close callback scope (Michael Dawson)
    #18089
* src:
  - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko)
    #17600
* vm:
  - add support for es modules (Gus Caplan)
    #17560

PR-URL: #18902
MylesBorins added a commit that referenced this pull request Feb 22, 2018
Notable changes:

* async_hooks:
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
  - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise
    (Ali Ijaz Sheikh) #18633
* deps:
  - update node-inspect to 1.11.3 (Jan Krems)
    #18354
  - ICU 60.2 bump (Steven R. Loomis)
    #17687
  - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems)
    #16889
* http:
  - add options to http.createServer() for `IncomingMessage` and
    `ServerReponse` (Peter Marton)
    #15752
* http2:
  - add http fallback options to .createServer (Peter Marton)
    #15752
* https:
  - Adds the remaining options from tls.createSecureContext() to the
    string generated by Agent#getName(). This allows https.request()
    to accept the options and generate unique sockets appropriately.
    (Jeff Principe)
    #16402
* inspector:
  - --inspect-brk for es modules (Guy Bedford)
    #18194
* lib:
  - allow process kill by signal number (Sam Roberts)
    #16944
* module:
  - enable dynamic import (Myles Borins)
    #18387
  - dynamic import is now supported (Jan Krems)
    #15713
* napi:
  - add methods to open/close callback scope (Michael Dawson)
    #18089
* src:
  - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko)
    #17600
* vm:
  - add support for es modules (Gus Caplan)
    #17560

PR-URL: #18902
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Notable changes:

* async_hooks:
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    nodejs#18513
  - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise
    (Ali Ijaz Sheikh) nodejs#18633
* deps:
  - update node-inspect to 1.11.3 (Jan Krems)
    nodejs#18354
  - ICU 60.2 bump (Steven R. Loomis)
    nodejs#17687
  - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems)
    nodejs#16889
* http:
  - add options to http.createServer() for `IncomingMessage` and
    `ServerReponse` (Peter Marton)
    nodejs#15752
* http2:
  - add http fallback options to .createServer (Peter Marton)
    nodejs#15752
* https:
  - Adds the remaining options from tls.createSecureContext() to the
    string generated by Agent#getName(). This allows https.request()
    to accept the options and generate unique sockets appropriately.
    (Jeff Principe)
    nodejs#16402
* inspector:
  - --inspect-brk for es modules (Guy Bedford)
    nodejs#18194
* lib:
  - allow process kill by signal number (Sam Roberts)
    nodejs#16944
* module:
  - enable dynamic import (Myles Borins)
    nodejs#18387
  - dynamic import is now supported (Jan Krems)
    nodejs#15713
* napi:
  - add methods to open/close callback scope (Michael Dawson)
    nodejs#18089
* src:
  - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko)
    nodejs#17600
* vm:
  - add support for es modules (Gus Caplan)
    nodejs#17560

PR-URL: nodejs#18902
MylesBorins pushed a commit that referenced this pull request May 23, 2018
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Original commit message:

    Introduce ScriptOrModule and HostDefinedOptions

    This patch introduces a new container type ScriptOrModule which
    provides the name and the host defined options of the script/module.

    This patch also introduces a new PrimitivesArray that can hold
    Primitive values, which the embedder can use to store metadata.

    The HostDefinedOptions is passed to V8 through the ScriptOrigin, and
    passed back to the embedder through HostImportModuleDynamically for
    module loading.

    Bug: v8:5785, v8:6658, v8:6683
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a
    Reviewed-on: https://chromium-review.googlesource.com/622158
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47724}

Backport-PR-URL: #17823
PR-URL: #16889
Refs: v8/v8@dbfe4a4
Refs: #15713
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants